Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add EnableMultipleHttp2Connections property to HttpHandlerOptions #2126

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Add EnableMultipleHttp2Connections property to HttpHandlerOptions #2126

wants to merge 4 commits into from

Conversation

EngRajabi
Copy link
Contributor

@EngRajabi EngRajabi commented Jul 21, 2024 •
edited by raman-m
Loading

If you activate HTTP/2, it is better to activate the SocketsHttpHandler.EnableMultipleHttp2Connections configuration as well, which gives good results.

Docs

@raman-m raman-m self-assigned this Jul 22, 2024
@raman-m
Copy link
Member

raman-m commented Jul 22, 2024 •
edited
Loading

Thank you, Mohsen!
Is prioritization required for this PR?
It seems straightforward, and we could aim to deliver it in the release following the 23.3 Hotfixes.

Currently, there is a shortage of both unit and acceptance tests to begin the review process. We need to implement a genuine HTTP/2 + The Option acceptance test to assess the stability of Ocelot Core in this scenario.

@raman-m raman-m added feature A new feature Configuration Ocelot feature: Configuration Routing Ocelot feature: Routing Oct'24 October 2024 release labels Jul 22, 2024
@raman-m raman-m added this to the Summer'24 milestone Jul 22, 2024
@raman-m raman-m changed the title feat: add EnableMultipleHttp2Connections Add EnableMultipleHttp2Connections property to HttpHandlerOptions Jul 22, 2024
@EngRajabi
Copy link
Contributor Author

EngRajabi commented Jul 23, 2024 •
edited by raman-m
Loading

It is not a high priority.
I can get a load test for it. But it is time consuming

raman-m
raman-m reviewed Jul 23, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestions are below 👇

docs/features/configuration.rst Show resolved Hide resolved
docs/features/configuration.rst Show resolved Hide resolved
src/Ocelot/Configuration/Creator/HttpHandlerOptionsCreator.cs
Comment on lines +28 to +29
options.UseCookieContainer, useTracing, options.UseProxy, maxConnectionPerServer, pooledConnectionLifetime,
options.EnableMultipleHttp2Connections);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly, extending the default advanced initialization constructor with all initialization values is beneficial. However, it's also worth considering the introduction of a copy constructor.

Having the copy-constructor we can write:

            return new HttpHandlerOptions(options);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raman-m
How to transfer the conditions of lines 21, 24 and 25? Line 21 uses ITracer

src/Ocelot/Configuration/File/FileHttpHandlerOptions.cs Show resolved Hide resolved
src/Ocelot/Configuration/HttpHandlerOptions.cs
@@ -6,14 +6,15 @@
public class HttpHandlerOptions
{
public HttpHandlerOptions(bool allowAutoRedirect, bool useCookieContainer, bool useTracing, bool useProxy,
int maxConnectionsPerServer, TimeSpan pooledConnectionLifeTime)
int maxConnectionsPerServer, TimeSpan pooledConnectionLifeTime, bool enableMultipleHttp2Connections)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lack of copy-constructor.

test/Ocelot.UnitTests/Configuration/DownstreamRouteExtensionsTests.cs
@@ -22,7 +22,7 @@ public DownstreamRouteExtensionsTests()
new List<DownstreamHostAndPort>(),
null,
null,
new HttpHandlerOptions(false, false, false, false, 0, TimeSpan.Zero),
new HttpHandlerOptions(false, false, false, false, 0, TimeSpan.Zero, false),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to define default constructor with default initialization values including a value for EnableMultipleHttp2Connections which is false?

test/Ocelot.UnitTests/Configuration/HttpHandlerOptionsCreatorTests.cs
.BDDfy();
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lack of Traits

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand what you mean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean this

Ocelot/test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs

Line 131 in ce0227c

[Trait("Issue", "1833")]

RaynaldM
RaynaldM approved these changes Aug 21, 2024
View reviewed changes
src/Ocelot/Configuration/HttpHandlerOptions.cs Outdated Show resolved Hide resolved
raman-m
raman-m requested changes Aug 30, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

  • Fix code review issues
  • Add unit tests
  • Add acceptance tests

@raman-m raman-m mentioned this pull request Aug 31, 2024
Open
@raman-m
Copy link
Member

raman-m commented Oct 18, 2024

@RaynaldM, why did you approve the PR? It lacks new unit tests and doesn't include any acceptance tests. Additionally, the issues I raised during the code review remain unaddressed.
Does your approval indicate that your suggestions have been implemented and you do not plan to review or enhance it further? Is that the reason for your approval?

@RaynaldM
Copy link
Collaborator

@raman-m I don't materially have the time to explore the PRs in depth, I'm content with a surface review, more on form than substance, and so my approval is on form.

@raman-m raman-m added Autumn'24 Autumn 2024 release and removed Oct'24 October 2024 release labels Oct 26, 2024
@raman-m raman-m modified the milestones: October'24, Autumn'24 Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raman-m raman-m raman-m requested changes

@ibnuda ibnuda ibnuda left review comments

@RaynaldM RaynaldM RaynaldM approved these changes

@ggnaegi ggnaegi Awaiting requested review from ggnaegi

Requested changes must be addressed to merge this pull request.

Assignees

@EngRajabi EngRajabi

@raman-m raman-m

Labels
Autumn'24 Autumn 2024 release Configuration Ocelot feature: Configuration feature A new feature Routing Ocelot feature: Routing
Projects
None yet
Milestone
Autumn'24
Development

Successfully merging this pull request may close these issues.
5 participants
@EngRajabi @raman-m @RaynaldM @ibnuda @ggnaegi